Extend support for logging a collection #7771
Conversation
carmocca
requested review from
Borda,
SeanNaren,
awaelchli,
justusschock,
kaushikb11,
tchaton and
williamFalcon
as code owners
carmocca
changed the title
Codecov Report
@@ Coverage Diff @@
## master #7771 +/- ##
======================================
- Coverage 93% 92% -0%
======================================
Files 199 199
Lines 12985 12979 -6
======================================
- Hits 12018 11978 -40
- Misses 967 1001 +34 |
justusschock
left a comment
justusschock
left a comment
There was a problem hiding this comment.
LGTM in general. There are a few typing things that are unrelated to this PR, but since it's a rather small one, I think it should be fine.
| def log_dict( | ||
| self, | ||
| dictionary: dict, | ||
| dictionary: Dict[str, _METRIC_COLLECTION], |
There was a problem hiding this comment.
The type here is interesting.
If this is read-only, let's use Mapping[str, _METRIC_COLLECTION] which is covarient with the value type of the key (meaning that Mapping[str, torch.Tensor] and Mapping[str, Metric]`) are both subtypes, and will validate type checking.
Since Dict implies mutability (eg, the function might mutate the dictionary), the type checker defines it as invariant. As such, Dict[str, torch.Tensor] is NOT a valid type to pass to to a function accepting Dict[str, Union[torch.Tensor, Metric]]). See here for discussion: python/mypy#2300 (comment)
There was a problem hiding this comment.
Interesting! I did not know that.
Feel free to open a patch with the change. Thanks!
There was a problem hiding this comment.
Since this is a minor type-only change, sent it out with #7851.
7 participants
What does this PR do?
Includes part of the changes to properly support
self.log('foo', {...}):Nonelog valuesDocumentation and tests for this feature will come in later PRs
Part of #7631
Before submitting
PR review